Skip to content

Fix #75917: SplFileObject::seek broken with CSV flags. #7697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

Flashwade1990
Copy link
Contributor

No description provided.

@cmb69 cmb69 added the Bug label Nov 29, 2021
@cmb69
Copy link
Member

cmb69 commented Nov 30, 2021

Thank you for the PR! However, it seems this is rather fixing the symptoms than curing the disease, namely that normal reading frees the line before checking for EOF

spl_filesystem_file_free_line(intern);
if (php_stream_eof(intern->u.file.stream)) {

but CSV reading does not
if (php_stream_eof(intern->u.file.stream)) {

What about the following patch instead:

 ext/spl/spl_directory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c
index c7dd8b4721..045aad5bc1 100644
--- a/ext/spl/spl_directory.c
+++ b/ext/spl/spl_directory.c
@@ -1939,6 +1939,8 @@ static int spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_obje
 
 	/* 1) use fgetcsv? 2) overloaded call the function, 3) do it directly */
 	if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || intern->u.file.func_getCurr->common.scope != spl_ce_SplFileObject) {
+		spl_filesystem_file_free_line(intern);
+
 		if (php_stream_eof(intern->u.file.stream)) {
 			if (!silent) {
 				zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot read from file %s", intern->file_name);

@Flashwade1990
Copy link
Contributor Author

Thank you for the PR! However, it seems this is rather fixing the symptoms than curing the disease, namely that normal reading frees the line before checking for EOF

spl_filesystem_file_free_line(intern);
if (php_stream_eof(intern->u.file.stream)) {

but CSV reading does not

if (php_stream_eof(intern->u.file.stream)) {

What about the following patch instead:

 ext/spl/spl_directory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c
index c7dd8b4721..045aad5bc1 100644
--- a/ext/spl/spl_directory.c
+++ b/ext/spl/spl_directory.c
@@ -1939,6 +1939,8 @@ static int spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_obje
 
 	/* 1) use fgetcsv? 2) overloaded call the function, 3) do it directly */
 	if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || intern->u.file.func_getCurr->common.scope != spl_ce_SplFileObject) {
+		spl_filesystem_file_free_line(intern);
+
 		if (php_stream_eof(intern->u.file.stream)) {
 			if (!silent) {
 				zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot read from file %s", intern->file_name);

Thank you for the PR! However, it seems this is rather fixing the symptoms than curing the disease, namely that normal reading frees the line before checking for EOF

spl_filesystem_file_free_line(intern);
if (php_stream_eof(intern->u.file.stream)) {

but CSV reading does not

if (php_stream_eof(intern->u.file.stream)) {

What about the following patch instead:

 ext/spl/spl_directory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c
index c7dd8b4721..045aad5bc1 100644
--- a/ext/spl/spl_directory.c
+++ b/ext/spl/spl_directory.c
@@ -1939,6 +1939,8 @@ static int spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_obje
 
 	/* 1) use fgetcsv? 2) overloaded call the function, 3) do it directly */
 	if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || intern->u.file.func_getCurr->common.scope != spl_ce_SplFileObject) {
+		spl_filesystem_file_free_line(intern);
+
 		if (php_stream_eof(intern->u.file.stream)) {
 			if (!silent) {
 				zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot read from file %s", intern->file_name);

Thank you for checking @cmb69, the suggested approach is much better. I update the PR.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks good to me, but I'd like a third pair of eyes reviewing this (after all, it's SPL …)

@cmb69 cmb69 closed this in daf79e2 Dec 6, 2021
@cmb69
Copy link
Member

cmb69 commented Dec 6, 2021

No further review, so I double-checked myself, and merged! Thanks again for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants